-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore existing package hashes for providers lock
command
#31389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks like a good approach to me! I just had one question inline where I wasn't sure if I was understanding a particular part of the logic correctly.
My uncertainty there also made me think about how it might've given me a little more to go on if our TestProvidersLock
also included some testing that the command's output contains the particular messages we're expecting, so I could use it to help set my expectations about what the implementation ought to be achieving. We don't always test against the human-oriented command output since of course it can be kinda brittle, but in this situation where the logic is embedded in the command implementation itself I think it can be pragmatic to do some substring testing as long as we're careful to make the test cases distinct enough to avoid false positives in the case of future regressions.
Alternatively, perhaps it would help to factor out that logic into a separate function that takes oldLock
and platformLock
and returns one of three values to direct the UI about which message it should show, and then we could test that helper function directly and not worry about testing the UI output because the logic in the command will presumably then just be a relatively simple switch
statement, or similar. 🤔
These are all just some ideas. If you have a different direction in mind then that's fine too! Let me know if I seem to be missing the point entirely! 😀
internal/command/providers_lock.go
Outdated
continue | ||
} | ||
|
||
if oldLock.ContainsAll(platformLock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of this call and the function it's calling is that it will return true
only if platformLock
doesn't add any new hashes compared to oldLock
, which feels opposite to the message it's printing. Am I misunderstanding the behavior of ContainsAll
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! I had my logic the wrong way, I've fixed it now. I followed the second idea, so split this out into a function and added some tests. Left a simple switch statement in the original function.
internal/depsfile/locks_test.go
Outdated
if !original.ContainsAll(target) { | ||
t.Fatalf("orginal should contain all hashes in target") | ||
} | ||
if target.ContainsAll(original) { | ||
t.Fatalf("target should not contain all hashes in orginal") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: this seems like a good opportunity to use t.Errorf
to allow both of these to potentially fail together if the function really misbehaves, since these two checks seem independent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
I left a little style nit inline that I'm sorry I didn't catch the first time (or maybe it wasn't there the first time? I'm not sure! 😀) but no need for another code review round-trip just for that.
"github.com/hashicorp/terraform/internal/addrs" | ||
"github.com/hashicorp/terraform/internal/depsfile" | ||
"github.com/hashicorp/terraform/internal/getproviders" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like what our usual tools would generate, but I must admit I'm not entirely sure what usual tool is responsible for it, and since the checks all passed I guess it might be something we have in our local editors only rather than something enforced by our CI process. 😖
Our typical basic style here is to separate the standard library packages from the external ones, with the standard library ones first and then a blank line followed by the external ones, with each separate "block" of packages in lexical order. I'd suggest manually changing this to match that for now, and then separately perhaps we can figure out which of the tools we're missing from our CI checks to catch this automatically in the future.
For some files that have a lot of imports we do also sometimes split the Terraform-internal packages from other external packages, so there would then be three "blocks" of packages: standard library, external, and then Terraform-internal. However, that's a more subjective call that our tooling does not enforce, and this one doesn't seem like it has enough packages to warrant that amount of fussiness, so I'm mentioning it only in the interests of helping you to see the patterns in other files in future. 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "default" grouping for imports is whatever goimports
creates automatically, which is usually bundled with the standard group of editor plugins. The CI checks only verify what go fmt
produces, which does not re-order imports on its own. It would be good to get that tool (or equivalent) working, since anyone else opening the file is going to rewrite it in their next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! And yeah it looks like goimports isn't installed by Goland automatically: https://www.jetbrains.com/help/go/integration-with-go-tools.html#goimports
I have now configured it.
Brief sidebar into/shoutout for https://pre-commit.com/ as something I've used in the past to make sure CI/CD and pre-commit checks on engineers machines are consistent (works across IDEs/architectures, forces common versions for all tools, etc).
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR addresses part of the concerns raised in #29958
First, the
terraform providers lock
command will now ignore any existing hashes in the.terraform.lock.hcl
file. It will retrieve new hashes as if there were none already logged and then merge any new hashes into the existing list. There is now no validation of any existing hashes. It does this with the introduction of theInstallNewProvidersForce
InstallMode
, which is set withinproviders_lock.go
.Second, the output of the
terraform providers lock
has been made more verbose. There are three cases tracked for each (provider,platform) pair requested:Full example output: